commit_ret_elems_ performance improvement and enclose basic types with namespace #142

sheepgrass · 2020-10-15T06:17:15Z

I've made the following changes:

improve performance by reducing commit_ret_elems_lock_ scope
improve performance by using find() instead of loop for getting entry from commit_ret_elems_
enclose typedefs with namespace in basic_type.hxx to avoid name collision with other projects
add skip_initial_election_timeout_ option to raft_params
support setting raft callback in raft_launcher::init()

…find() instead of loop for getting entry from commit_ret_elems_

greensky00

Hi @sheepgrass, thanks for your contributions.

I see that your current PR contains multiple changes together, and we recommend one (functional) change per PR. I think it can be split into three PRs:

Adding namespace nuraft around custom type definitions.
Passing skip_initial_election_timeout_ and callback functions to Raft launcher.
commit_ret_elems_ things.

Could you please submit separate PRs for 1 and 2? And please refer to my comments regarding 3. Thanks!

greensky00 · 2020-10-19T17:50:46Z

CMakeLists.txt

@@ -34,7 +34,7 @@ if (BOOST_INCLUDE_PATH AND BOOST_LIBRARY_PATH)

    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DUSE_BOOST_ASIO")

-    set(ASIO_INCLUDE_DIR ${BOOST_INCLUDE_PATH})
+    set(ASIO_INCLUDE_DIR ${BOOST_INCLUDE_PATH}/boost)


This line is unnecessary, you can set the variable as

-DBOOST_INCLUDE_PATH=<your library include path>/boost

As said this is my first PR, actually this cmake file change is for my own use only and I actually have no intention to make PR for this. Sorry for bother you.

greensky00 · 2020-10-19T17:52:50Z

CMakeLists.txt

+if (BOOST_INCLUDE_PATH)
+    message(STATUS "Boost include path: " ${BOOST_INCLUDE_PATH})
+    include_directories(BEFORE ${BOOST_INCLUDE_PATH})
+endif ()


Boost include path is already printed out at line 32 above. And also include_directories of boost include path is not required, as what we need is ASIO_INCLUDE_DIR only.

As said this is my first PR, actually this cmake file change is for my own use only and I actually have no intention to make PR for this. Sorry for bother you.

greensky00 · 2020-10-19T17:53:14Z

include/libnuraft/basic_types.hxx

@@ -23,6 +23,8 @@ limitations under the License.

 #include <cstdint>

+namespace nuraft {


Can you please make this change as a separate PR? Thanks.

greensky00 · 2020-10-19T17:57:18Z

include/libnuraft/raft_params.hxx

+
+    /**
+     * If `true`, the election timer will not be initiated
+     * automatically, so that this node will never trigger
+     * leader election until it gets the first heartbeat
+     * from any valid leader.
+     *
+     * Purpose: to avoid becoming leader when there is only one
+     *          node in the cluster.
+     */
+    bool skip_initial_election_timeout_;


raft_params includes parameters repeatedly used throughout the lifetime of a Raft server, while skip_initial_election_timeout_ is just a one-time parameter for the initialization only. If the reason why you added it here is merely for passing it to launcher, please add raft_server::init_options as a parameter of the constructor of launcher.

understood and noted

greensky00 · 2020-10-19T17:58:12Z

include/libnuraft/launcher.hxx

@@ -45,7 +46,8 @@ public:
                          ptr<logger> lg,
                          int port_number,
                          const asio_service::options& asio_options,
-                          const raft_params& params);
+                          const raft_params& params,
+                          cb_func::func_type raft_callback = nullptr);


As I mentioned in the other comment, can you please replace this line with raft_server::init_options, and move cb_func::func_type raft_callback into raft_server::init_options? Please make this change as a separate commit too.

understood and noted

greensky00 · 2020-10-19T18:30:49Z

src/handle_commit.cxx

-        auto entry = commit_ret_elems_.begin();
-        while (entry != commit_ret_elems_.end()) {
+        auto entry = commit_ret_elems_.find(sm_idx);
+        if (entry != commit_ret_elems_.end()) {
            ptr<commit_ret_elem> elem = entry->second;


Changes regarding commit_ret_elems_lock_ will not be acceptable because of the following situation. Let's say T1 is the user thread (calling append_entries) and T2 is the commit thread.

T1: appends log and calls pre_commit, but thread scheduling happens before dealing with commit_ret_elem.

Replication is done.

T2: calls commit_app_log, grabs the lock, but cannot find corresponding commit_ret_elem, and releases the lock.

T1: grabs the lock, adds commit_ret_elem (let's say elem1) as there is no corresponding commit_ret_elem, and releases the lock.

T2: grabs the lock, adds commit_ret_elem (let's say elem2), and invokes elem2.

T1: sleeps using elem1.

Due to the racing between user and commit threads, commit_ret_elems_lock_ should protect the entire process of 1) finding commit_ret_elem and 2) inserting commit_ret_elem if not exists. We can't release the lock in the middle.

Please note that, in most cases, there won't be notable performance degradation caused by commit_ret_elems_lock_, as Raft commit rate is around a few thousand operations per second due to network latency and disk I/O (by log store and state machine), while the commit_ret_elem logic within the lock is a pure in-memory operation without I/O: faster than a few hundred thousand executions per second I believe. The impact will be marginal.

Actually the changes regarding commit_ret_elems_lock_ have been made a few months ago so I have already forgotten some details here. But my main purpose is to avoid locking unnecessary scope to reduce lock contention.

And for commit_app_log(), I want to avoid elem->awaiter_.invoke() inside commit_ret_elems_lock_ as it would wait other thread to gain lock inside.

Besides the lock, for commit_app_log() function, I have removed the while loop for linear search and instead I use the commit_ret_elems_.find() for getting the required entry. Please review if it's ok.

sheepgrass · 2020-10-20T01:30:59Z

Hi @sheepgrass, thanks for your contributions.

I see that your current PR contains multiple changes together, and we recommend one (functional) change per PR. I think it can be split into three PRs:

Adding namespace nuraft around custom type definitions.

Passing skip_initial_election_timeout_ and callback functions to Raft launcher.

commit_ret_elems_ things.

Could you please submit separate PRs for 1 and 2? And please refer to my comments regarding 3. Thanks!

Sorry for mixing the issues, since this is my first PR so I don't know the common practice for that. I will create separate PRs.

Curtis Lo added 3 commits October 15, 2020 14:03

improve performance by reducing commit_ret_elems_lock_ scope and use …

2cafb41

…find() instead of loop for getting entry from commit_ret_elems_

enclose typedefs with namespace in basic_type.hxx

dfdca71

add skip_initial_election_timeout_ option to raft_params

5afc198

sheepgrass mentioned this pull request Oct 15, 2020

move skip_initial_election_timeout_ option to raft_params #144

Closed

support setting raft callback in raft_launcher::init()

aeb70ea

greensky00 reviewed Oct 19, 2020

View reviewed changes

sheepgrass marked this pull request as draft October 20, 2020 09:02

sheepgrass closed this Oct 22, 2020

sheepgrass deleted the master branch October 22, 2020 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commit_ret_elems_ performance improvement and enclose basic types with namespace #142

commit_ret_elems_ performance improvement and enclose basic types with namespace #142

sheepgrass commented Oct 15, 2020 •

edited

Loading

greensky00 left a comment

greensky00 Oct 19, 2020

sheepgrass Oct 20, 2020

greensky00 Oct 19, 2020

sheepgrass Oct 20, 2020

greensky00 Oct 19, 2020

sheepgrass Oct 20, 2020

greensky00 Oct 19, 2020

sheepgrass Oct 20, 2020

greensky00 Oct 19, 2020 •

edited

Loading

sheepgrass Oct 20, 2020

greensky00 Oct 19, 2020

sheepgrass Oct 20, 2020

sheepgrass commented Oct 20, 2020

		@@ -23,6 +23,8 @@ limitations under the License.

		#include <cstdint>

		namespace nuraft {

commit_ret_elems_ performance improvement and enclose basic types with namespace #142

commit_ret_elems_ performance improvement and enclose basic types with namespace #142

Conversation

sheepgrass commented Oct 15, 2020 • edited Loading

greensky00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greensky00 Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheepgrass commented Oct 20, 2020

sheepgrass commented Oct 15, 2020 •

edited

Loading

greensky00 Oct 19, 2020 •

edited

Loading